do not call llm on empty prompt #614
Conversation
pelikhan
commented
Aug 12, 2024
•
pelikhan
commented
- The updated code introduces an additional condition in a logical check within the 'expandTemplate' function, in the 'expander.ts' file of the core package. This piece of code handles responses to prompts.
- Previously, the code only checked if 'prompt.status' was not equal to "success".
- Now, it also checks whether 'prompt.text' is an empty string or not. The check will pass if either 'prompt.status' is not "success" or 'prompt.text' is an empty string.
- This means the function will now return early not only when the prompt status is unsuccessful, but also when the prompt text is empty 🧐.
- This change could be handling a case where the status is successful but no prompt text is provided, avoiding subsequent operations on an empty text 👍.
|
The change in the pull request adds an additional check to the if condition. Previously, it was checking if This change seems to add an additional layer of validation, ensuring that not only the status should be success but also the However, please be aware that this could potentially break any functionality that depends on this function and expects it to return a status of success even when If you are confident that this won't break any dependent functionality or you have handled those cases, then this change looks good to me. So, "LGTM 🚀" provided the potential impact on dependent functionality has been considered and appropriately handled.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -220,7 +220,7 @@ export async function expandTemplate( | |||
| if (prompt.aici) trace.fence(prompt.aici, "yaml") | |||
| trace.endDetails() | |||
|
|
|||
| if (prompt.status !== "success") | |||
| if (prompt.status !== "success" || prompt.text === "") | |||
There was a problem hiding this comment.
The check for an empty string in the condition prompt.text === "" might not be necessary if prompt.status is not "success". If the status is not "success", it implies that the operation failed or was cancelled, so the text might not be relevant in this context. 🤔
generated by pr-review-commit
empty_string_check
| @@ -220,7 +220,7 @@ export async function expandTemplate( | |||
| if (prompt.aici) trace.fence(prompt.aici, "yaml") | |||
| trace.endDetails() | |||
|
|
|||
| if (prompt.status !== "success") | |||
| if (prompt.status !== "success" || prompt.text === "") | |||
There was a problem hiding this comment.
The logic of the condition has been changed with the addition of prompt.text === "". This might lead to different behavior of the code if prompt.text is an empty string. If this change was intentional, it would be good to add a comment explaining why this change was made. 😊
generated by pr-review-commit
logic_change
| @@ -220,7 +220,7 @@ export async function expandTemplate( | |||
| if (prompt.aici) trace.fence(prompt.aici, "yaml") | |||
| trace.endDetails() | |||
|
|
|||
| if (prompt.status !== "success") | |||
| if (prompt.status !== "success" || prompt.text === "") | |||
There was a problem hiding this comment.
The condition prompt.text === "" has been added without any comment or explanation. It would be helpful to add a comment explaining why this condition is necessary, to make the code easier to understand for other developers. 📝
generated by pr-review-commit
missing_comment
